Repository navigation
Migrate ShareActionHandlerModule to a type-safe Nitro module - #104550
nabi-ebrahimi wants to merge 16 commits into
Conversation
Generate one shared-file contract for Swift, Kotlin, and TypeScript. Keep native storage internal and preserve the existing Onyx timestamp format. Refs: Expensify#104432
|
|
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@shubham1206agra Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
| }; | ||
|
|
||
| interface ShareActionHandler extends HybridObject<{ios: 'swift'; android: 'kotlin'}> { | ||
| /** Returns an empty array when no share is pending; rejects if pending data cannot be read. */ |
There was a problem hiding this comment.
❌ CONSISTENCY-15 (docs)
This newly added JSDoc comment joins two independent clauses with a semicolon (...when no share is pending; rejects if pending data cannot be read.). Comments should read as plain, natural sentences; a semicolon used to splice two statements together should be split into two separate sentences.
Suggested fix:
/** Returns an empty array when no share is pending. Rejects if pending data cannot be read. */
processFiles(): Promise<SharedFile[]>;Reviewed at: e0ee644 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return [] | ||
| } | ||
|
|
||
| return try files.map { try self.processFile($0) } |
There was a problem hiding this comment.
(Shubham's Ai) This rejects the whole batch if any single file fails processFile (empty, unreadable, not a regular file, or non-UTF-8 text_to_read). The old ObjC module skipped bad files and returned the rest. Could we keep that behavior, e.g. compactMap with try? (logging the skipped file), and only reject if no usable file remains?
There was a problem hiding this comment.
Thanks, addressed. The iOS handler now processes each shared file independently, logs and skips files that fail processing, and returns any valid files. If every pending file fails, it still rejects so the existing attachment-error UI is displayed.
| const tempFile = processedFiles.at(0); | ||
| if (!tempFile) { | ||
| setErrorTitle(translate('attachmentPicker.attachmentError')); | ||
| setErrorMessage(translate('attachmentPicker.errorWhileSelectingCorruptedAttachment')); |
There was a problem hiding this comment.
(Shubham's Ai) An empty result now shows errorWhileSelectingCorruptedAttachment, but nothing is corrupt in this case; there's just no pending share. This path also runs whenever the app returns to the foreground (the AppState listener calls handleProcessFiles again). Is there an existing message that fits "nothing to share" better?
There was a problem hiding this comment.
Thanks, addressed. An initial empty result now displays the existing localized “Attachment not found” message instead of the corrupted-attachment copy. Empty results are ignored after a valid attachment has already loaded, preventing an incorrect alert when the app resumes. I also added an explicit local result type for the Share handler callback.
| return@parallel arrayOf(SharedFile("text", content, "txt", timestamp, null)) | ||
| } | ||
|
|
||
| if (!file.isFile || !file.canRead() || file.length() == 0L) { |
There was a problem hiding this comment.
(Shubham's Ai) Throwing on a zero-byte file (here and the size > 0 guard in the Swift implementation) means the user gets the generic attachment error instead of the existing "attachment too small" (sizeNotMet) message from ShareRootPage. Could we let empty files through and keep the JS size validation as the single source of that error?
There was a problem hiding this comment.
Thanks, addressed. Zero-byte regular files now reach the existing JavaScript size validation, restoring the sizeNotMet error for applicable file types. Native code continues to reject missing, unreadable, and non-regular files.
| } | ||
|
|
||
| const {fileExtension} = splitExtensionFromFileName(tempFile?.content); | ||
| if (tempFile) { |
There was a problem hiding this comment.
(Shubham's Ai) Nit: after the if (!tempFile) return guard above, this if (tempFile) is always true and the tempFile?. optional chains in this block are redundant. This could be flattened.
There was a problem hiding this comment.
Thanks, addressed. The missing-file guard already returns before this block, so I removed the redundant if (tempFile) check and optional chaining, and flattened the remaining shared-file processing without changing behavior.
| const handleProcessFiles = () => { | ||
| ShareActionHandler.processFiles() | ||
| .then((processedFiles) => { | ||
| if (errorTitle) { |
There was a problem hiding this comment.
(Shubham's Ai) errorTitle here is the value from the render that started the call, not the current one, so it is stale by the time the promise resolves. This existed before, but the async chain widens the gap. Consider a ref or at least a comment.
There was a problem hiding this comment.
Thanks, addressed. Shared-file processing now reads the current error state from a ref rather than the render-time closure. All Share errors go through a helper that updates the ref immediately, preventing a later async result from overwriting or duplicating an existing error.
|
|
||
| override fun processFiles(): Promise<Array<SharedFile>> = Promise.parallel { | ||
| val context = NitroModules.applicationContext ?: throw IOException("React Native context is unavailable") | ||
| val preferences = context.getSharedPreferences("shareActionHandler", Context.MODE_PRIVATE) |
There was a problem hiding this comment.
(Shubham's Ai) Nit: "shareActionHandler" / "shareObject" duplicate IntentHandlerConstants (in both App and Mobile-Expensify), which this module can't import. Could you add a comment pointing at the source so the two don't drift?
There was a problem hiding this comment.
Thanks, addressed. I added a comment identifying IntentHandlerConstants as the source of truth for the duplicated preference file and key, and explaining that the standalone Nitro module cannot import app-level Android code.
| retriever.release() | ||
| } | ||
| } | ||
| else -> null |
There was a problem hiding this comment.
(Shubham's Ai) aspectRatio is now undefined for non-media files on both platforms (previously 0 on Android, 1 on iOS). AttachmentPreview treats it as optional, so this looks fine, but please confirm a PDF share still previews correctly.
There was a problem hiding this comment.
Thanks for flagging. Confirmed by the existing preview implementation: when aspectRatio is undefined, AttachmentPreview defaults it to 1, and the PDF-specific preview branch also explicitly uses aspectRatio: 1. Therefore, PDF preview behavior is unaffected.
| NitroModules: {createHybridObject: jest.fn(() => ({processFiles: jest.fn()}))}, | ||
| })); | ||
|
|
||
| describe('ShareActionHandler native adapter', () => { |
There was a problem hiding this comment.
(Shubham's Ai) These tests stub processFiles and then assert that it returns the stubbed value, so they only exercise the mock. tests/ui/ShareRootPageTest.tsx covers the real behavior; I think this file can be dropped.
There was a problem hiding this comment.
Thanks, agreed. I removed the test because it only asserted values returned from a mocked processFiles() implementation. The ShareRootPage tests continue to cover handling of successful, empty, and rejected shared-file results.
| let content = isText ? try String(contentsOf: url, encoding: .utf8) : url.absoluteString | ||
| var aspectRatio: Double? | ||
|
|
||
| if mimeType.hasPrefix("image/"), let image = UIImage(contentsOfFile: url.path), image.size.height > 0 { |
There was a problem hiding this comment.
(Shubham's Ai) Optional: UIImage(contentsOfFile:) decodes the whole image just to read its dimensions. CGImageSourceCopyPropertiesAtIndex reads only the header, which is cheaper for large photos. (The old ObjC code did the same, so not a regression.)
There was a problem hiding this comment.
Thanks for the suggestion. This preserves the existing Objective-C behavior and runs off the UI thread inside Promise.async, so I’ll keep the current implementation in this migration PR. Using CGImageSourceCopyPropertiesAtIndex would be a worthwhile separate performance improvement for large images.
|
@nabi-ebrahimi Link https://github.com/Expensify/Mobile-Expensify/pull/14201 for HybridApp changes so we can make a build that works. |
|
@nabi-ebrahimi Bump here |
|
@shubham1206agra, I’ve addressed all your feedback. Please take another look when you have a chance. Thanks! |
|
@nabi-ebrahimi You have not completed #104550 (comment). Please do this step first |
@shubham1206agra, Thank you for pointing this out. However, I don't have access to the link you shared. Could you please let me know what I should do? I'd really appreciate your guidance. |
@nabi-ebrahimi Add this PR link to the description of this PR. It does not require any special access |
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Done! I have updated the PR description and linked the companion PR (https://github.com/Expensify/Mobile-Expensify/pull/14201) under ### Linked PRs as requested. Also, I’ve merged the latest main branch and resolved all conflicts. Please take a look whenever you have time. Thank you! |
|
@shubham1206agra, Gentle bump. thanks. |
Generate one shared-file contract for Swift, Kotlin, and TypeScript. Keep native storage internal and preserve the existing Onyx timestamp format.
Refs: #104432
Explanation of Change
This PR migrates the legacy
ShareActionHandlerReact Native bridge to a type-safe Nitro module within@expensify/nitro-utils.It replaces callback-based
NativeModulescommunication and platform-specific payload formats with a single generatedSharedFilecontract implemented in Kotlin for Android and Swift for iOS.processFiles()now returnsPromise<SharedFile[]>, eliminating Android JSON parsing and untyped native dictionaries in the Share flow, while safely falling back to an empty array on web.The native implementations retain existing share behavior for images, videos, PDFs, plain text, and
.txtfile attachments, while improving validation for missing, unreadable, empty, or malformed shared content. The Share screen now surfaces the existing attachment-error UI for empty results or native-processing rejections, preventing an endless loading skeleton.Fixed Issues
$ #104530
PROPOSAL:
Linked PRs
Tests
/and verify it is handled as text, not as a missing file path.Offline tests
.zip, and verify the existing wrong-file-type error appears while offline.QA Steps
same as test
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
REC-20261022115641.mp4
Android: mWeb Chrome
N/A
iOS: Native
REC-20261022123326.mp4
iOS: mWeb Safari
N/A
MacOS: Chrome / Safari
N/A